Land governed agents, Slack, and homelab release - #42
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds organisation-scoped agent execution, direct-message replies, a governed agent harness with Slack adapters, synthetic cleanup workflows, audit/readiness tooling, archived-record filtering, connector support, and immutable release/homelab validation. ChangesGoverned runtime and harness
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (!payload || !signature || !secret) | ||
| throw new Error("Invalid Slack OAuth state"); | ||
| const expected = createHmac("sha256", secret) | ||
| .update(payload) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/agent-gateway/src/runtime.ts (1)
1706-1718: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
request()stripsharness, so live connector evidence never executes.
PersistedRequestdeclaresharness?.mode, andloadLiveConnectorEvidencereturns early unlessrequest.harness?.mode === "slack"(Line 2226-2230). But this Zod object has noharnesskey, and Zod strips unknown keys by default, sothis.request(run).harnessis alwaysundefinedfor every caller — includingloadAuthoritativeContextat Line 929-933. The entire Tawny/Kelpie/UniFi grounding path added in this PR is unreachable.🐛 Proposed fix
humanRequest: z.string().optional(), sourceMessageId: z.uuid().optional(), traceId: z.string().optional(), + harness: z + .object({ + mode: z + .enum(["slack", "hermes", "mcp", "cli", "http"]) + .optional(), + }) + .optional(), })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/agent-gateway/src/runtime.ts` around lines 1706 - 1718, Update the Zod schema inside request() to parse and preserve the harness field defined by PersistedRequest, including its mode value, so this.request(run).harness?.mode remains available to loadLiveConnectorEvidence and loadAuthoritativeContext. Keep the existing validation and fallback behavior unchanged for other request fields and invalid payloads.apps/web/lib/connector-domain.ts (1)
74-97: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winArchived connectors remain fully operable despite being hidden from
list().
list()(Lines 46-49) now excludes archived integration records, butconfigure()'s existing-record lookup (Lines 74-87),queueQuery()'s integration lookup (Lines 245-257), androtate()'s credential update (Lines 185-214) all still match byorganisationId+ id/product/instanceId with noarchivedAtcheck. In practice:
configure()will silently "revive" an archived connector's row (settingstatus: "configured"again) without clearingarchivedAt, leaving it hidden from the UI forever while actively updated.queueQuery()will happily queue queries against a connector that's supposed to be archived/cleaned up.rotate()will rotate credentials for an archived connector.This undermines the governance intent behind archiving connector evidence (e.g. synthetic-cleanup).
🛡️ Proposed fix
const [existing] = await tx .select({ id: schema.integrationRecords.id }) .from(schema.integrationRecords) .where( and( eq( schema.integrationRecords.organisationId, subject.organisationId, ), eq(schema.integrationRecords.product, request.product), eq(schema.integrationRecords.instanceId, request.instanceId), + isNull(schema.integrationRecords.archivedAt), ), ) .limit(1);const [integration] = await this.db .select({ configuration: schema.integrationRecords.configuration, status: schema.integrationRecords.status, }) .from(schema.integrationRecords) .where( and( eq(schema.integrationRecords.organisationId, subject.organisationId), eq(schema.integrationRecords.id, integrationId), + isNull(schema.integrationRecords.archivedAt), ), ) .limit(1);
rotate()will additionally need a join or a preceding existence check againstintegrationRecordsfiltered onisNull(archivedAt)before updating credentials.Also applies to: 238-264, 185-236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/connector-domain.ts` around lines 74 - 97, Prevent archived integration records from being used by configure(), queueQuery(), and rotate(). Add an isNull(archivedAt) predicate to the existing-record lookups in configure() and queueQuery(), and make rotate() verify the associated integrationRecords row is unarchived before updating credentials; preserve the existing not-found behavior when no active record exists.
🟠 Major comments (29)
packages/database/migrations/0020_ancient_carnage.sql-81-96 (1)
81-96: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEnforce organisation affinity for Slack references.
These foreign keys validate each ID independently, so rows can mix an
organisation_idfrom tenant A with an installation, agent, actor, run, or inbox event from tenant B. For example, a cross-tenantslack_run_deliveriesrow could route a tenant B run through tenant A’s Slack installation. Use composite(organisation_id, id)keys and composite foreign keys (or equivalent database-enforced validation) for these relationships.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/migrations/0020_ancient_carnage.sql` around lines 81 - 96, Update the Slack-related schema and migration constraints for slack_agent_exposures, slack_identity_mappings, slack_inbox_events, slack_installations, and slack_run_deliveries so referenced entities are validated by composite (organisation_id, id) keys rather than independent IDs. Add matching composite unique keys where needed and replace the listed single-column foreign keys with composite foreign keys that enforce the same organisation across installations, agents, actors, runs, and inbox events.apps/web/app/api/v1/slack/events/route.ts-24-29 (1)
24-29: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSwallowed
recordEventfailure is unobservable and acks Slack retries.Returning
ok: trueafter an exception tells Slack the event was accepted, so its retry budget is consumed while nothing durable was written (a thrown transaction rolls back rather than recording health). Distinguish expected cases (unmapped workspace/duplicate) from unexpected ones, and at minimum emit a structured log so silent event loss is detectable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/api/v1/slack/events/route.ts` around lines 24 - 29, Update the error handling around SlackGovernanceAdapter.recordEvent to distinguish expected unmapped-workspace or duplicate-event failures from unexpected failures. Preserve acknowledgment for expected cases, but log unexpected errors with structured context before returning, ensuring transaction failures are observable without exposing details to Slack..env.example-33-35 (1)
33-35: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPlaceholder token passes the
min(32)validation.
apps/agent-gateway/src/index.tsonly enforcesz.string().min(32), and this 37-char placeholder satisfies it, so an unedited.envcopy yields a working gateway with a publicly known bearer token. Consider a shorter, obviously invalid placeholder (or empty value) so startup fails loudly.🔒 Suggested change
-MUSTER_AGENT_GATEWAY_TOKEN=replace-with-at-least-32-random-bytes +# Generate with: openssl rand -hex 32 +MUSTER_AGENT_GATEWAY_TOKEN=🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 33 - 35, Replace the MUSTER_AGENT_GATEWAY_TOKEN placeholder in the environment example with a shorter, clearly invalid value that cannot satisfy the current 32-character minimum validation, while preserving the variable and surrounding configuration.apps/web/lib/agent-learning-domain.ts-831-907 (1)
831-907: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftDirect-message reply projection is duplicated from the gateway.
This block reproduces
projectDirectMessageTerminalReplyinapps/agent-gateway/src/runtime.ts:86-207— same source-message/room/membership guards, same document shape, and the sameagent-direct-message-reply:${run.id}/room.message.created:agent-direct-message:${run.id}keys. The two will drift. Consider hoisting the projection into a shared package (e.g.@muster/databaseor a shared domain module) consumed by both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/agent-learning-domain.ts` around lines 831 - 907, Extract the duplicated direct-message terminal reply projection into a shared domain/database helper, then replace this inline transaction logic and gateway implementation in projectDirectMessageTerminalReply with that helper. Preserve the existing source-message, room, membership, document, idempotency-key, and outbox behavior while ensuring both callers use the same implementation.apps/web/app/api/v1/slack/commands/route.ts-22-39 (1)
22-39: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSilent catch drops the command with no signal to operator or user.
Every failure from
recordEvent— including "Slack workspace is not connected" and encryption-key errors — is swallowed with no log, and the user is still told "Muster accepted your request." Nothing durable was written in that path (the outbox write happens insiderecordEvent), so there is no retry either. At minimum log the failure and differentiate the acknowledgement text.🐛 Proposed fix
try { await new SlackGovernanceAdapter().recordEvent(rawBody, { ... }); - } catch { - // The signed request is acknowledged within Slack's deadline; outbox retry - // and installation health hold the durable failure path. - } - return Response.json({ response_type: "ephemeral", text: "Muster accepted your request." }); + } catch (error) { + console.error( + JSON.stringify({ + level: "error", + event: "slack.command.record_failed", + teamId, + error: error instanceof Error ? error.message : "unknown", + }), + ); + return Response.json({ + response_type: "ephemeral", + text: "Muster could not accept your request. Contact an operator if this continues.", + }); + } + return Response.json({ + response_type: "ephemeral", + text: "Muster accepted your request.", + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/api/v1/slack/commands/route.ts` around lines 22 - 39, Update the error handling around SlackGovernanceAdapter.recordEvent to log the caught failure with sufficient context for operators, and track whether recording succeeded. Return the existing “Muster accepted your request.” acknowledgement only on success; when recordEvent fails, return a distinct ephemeral response indicating the request could not be recorded instead of claiming acceptance.apps/web/app/api/v1/agent-harness/invocations/route.ts-8-15 (1)
8-15: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNamespace the client-supplied idempotency key.
The raw header value is written straight into
agentRuns.idempotencyKey, which is a shared namespace within an organisation (e.g.parker-agent-run:*inapps/web/lib/parker-report-domain.ts,integration:*). A caller can pass a key that collides with a run created by another producer;GovernedAgentHarness.invoke(packages/agent-harness/src/index.ts:155-316) would then resolve that unrelated run and return it as a duplicate. Prefix the key and bound its length.🛡️ Proposed fix
- const idempotencyKey = request.headers.get("idempotency-key")?.trim(); - if (!idempotencyKey) + const suppliedKey = request.headers.get("idempotency-key")?.trim(); + if (!suppliedKey || suppliedKey.length > 160) throw new Error("Idempotency-Key header is required for harness invocations"); + const idempotencyKey = `agent-harness:${suppliedKey}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/api/v1/agent-harness/invocations/route.ts` around lines 8 - 15, Update the idempotency key handling in the route before calling GovernedAgentHarness.invoke: namespace the trimmed client-supplied value with a harness-specific prefix and enforce the supported maximum length. Pass only this namespaced, bounded key to invoke so it cannot collide with keys from other producers, while preserving the required-header validation.apps/agent-gateway/src/runtime.ts-1218-1252 (1)
1218-1252: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHeartbeat progress writes need a unique or conflict-tolerant idempotency key
outbox_events.idempotency_keyis uniquely indexed, and this heartbeat reusesagent.run.progress:${run.id}:executingon every interval. The second renewal will reject inside the transaction, and the interval callback does not catch that rejection, so lease renewal stops for an otherwise healthy run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/agent-gateway/src/runtime.ts` around lines 1218 - 1252, Update the heartbeat transaction in the lease-renewal method containing the agentRuns update and writeOutbox call so each progress event uses a unique or conflict-tolerant idempotencyKey instead of reusing `agent.run.progress:${run.id}:executing`. Preserve the existing outbox payload and progress behavior while ensuring repeated heartbeat intervals cannot reject the transaction due to the unique `outbox_events.idempotency_key` constraint.apps/web/app/api/v1/slack/oauth/callback/route.ts-10-44 (1)
10-44: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCallback authorises solely from the signed state; no session binding.
The subject is reconstructed from
statealone, so anyone who obtains the callback URL (browser history, referrer, logs, shared link) within the 10-minute window can complete the installation as the encoded actor.consumeOAuthStateprevents replay after the first use, but not first use by a third party. Consider also verifying the current session matchesverified.actorId/organisationIdwhen a session is present, and falling back to failure otherwise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/api/v1/slack/oauth/callback/route.ts` around lines 10 - 44, Bind the Slack OAuth callback to the authenticated session before authorising the installation: after verifySlackOAuthState in GET, retrieve the current session and require it to identify the same actor and organisation as verified.actorId and verified.organisationId. Reject the callback when no session exists or the session does not match, then continue constructing the subject and calling consumeOAuthState only after this validation.packages/agent-harness/src/index.ts-1377-1382 (1)
1377-1382: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftNo atomic claim on the inbox event; concurrent workers can double-post.
The status check is a read followed by a much later write (TOCTOU). Two workers handed the same
inboxEventId(outbox retry + Socket Mode reconnect) both pass this guard. The harnessidempotencyKeyprevents a second agent run, butchat.postMessageat Line 1713 runs in both paths, so the user sees duplicate acknowledgements. Claim the row atomically first, e.g.UPDATE slack_inbox_events SET status='processing' WHERE id = ? AND status IN ('pending','failed') RETURNING *, and bail when no row is returned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-harness/src/index.ts` around lines 1377 - 1382, Replace the read-only status guard in the inbox event handling flow with an atomic claim using the event row’s update operation: transition the matching slack inbox event to processing only when its current status is pending or failed, and retrieve the claimed row. Return immediately when no row is claimed, then use the returned row for subsequent processing so concurrent workers cannot both reach chat.postMessage.packages/agent-harness/src/index.ts-411-423 (1)
411-423: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winState replay guard can be bypassed by appending an extra dot-segment.
value.split(".")keeps only the first two parts, sopayload.signature.junkverifies successfully, whileconsumeOAuthStatehashes the whole raw string — producing a different idempotency key each time. A caller can therefore reuse one valid state indefinitely (within TTL) despite the single-use record. Reject anything that isn't exactly two segments.🔒 Proposed fix
- const [payload, signature] = value.split("."); + const parts = value.split("."); + const [payload, signature] = parts; const secret = process.env.SLACK_OAUTH_STATE_SECRET; - if (!payload || !signature || !secret) + if (parts.length !== 2 || !payload || !signature || !secret) throw new Error("Invalid Slack OAuth state");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-harness/src/index.ts` around lines 411 - 423, Update verifySlackOAuthState to reject values that do not contain exactly one dot and therefore exactly two non-empty segments before validating the signature. Ensure the payload and signature used for HMAC verification match the complete raw state consumed by consumeOAuthState, while preserving the existing invalid-state error behavior.apps/web/app/api/v1/slack/interactions/route.ts-18-24 (1)
18-24: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSilently swallowed errors make dropped interactions invisible.
If
recordEventthrows before the inbox insert (unknown/inactive workspace, encryption or transaction failure), nothing is persisted — so the "durable inbox carries retries" claim doesn't hold for this path, and there is no log or metric to detect it. Keep the fast 200 to Slack, but emit a structured error log.🔭 Proposed fix
try { const payload = JSON.parse(encoded) as Record<string, unknown>; await new SlackGovernanceAdapter().recordEvent(rawBody, payload); - } catch { + } catch (error) { // Slack requires a fast acknowledgement. The durable inbox carries retries // and operator-visible delivery health without exposing internals to Slack. + console.error( + JSON.stringify({ + level: "error", + event: "slack.interaction.record_failed", + error: error instanceof Error ? error.message : "unknown", + }), + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/api/v1/slack/interactions/route.ts` around lines 18 - 24, Update the catch block in the Slack interaction handler around SlackGovernanceAdapter.recordEvent to emit a structured error log containing the caught error and relevant interaction context, while preserving the immediate successful acknowledgement to Slack and existing behavior for JSON parsing and event recording.apps/web/app/api/v1/ready/route.ts-1-16 (1)
1-16: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMove
readinessResponseout ofroute.tsNext route files should only export handlers/config, so this extra named export can fail route export validation. Put it inapps/web/lib/readiness.tsand import it from the route and test (prefer@/lib/readiness).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/api/v1/ready/route.ts` around lines 1 - 16, Move the readinessResponse function from the route module into the readiness library, then export and reuse it there. Update the GET handler in the route and any tests to import readinessResponse from the library, preferably through the `@/lib/readiness` alias, leaving the route to export only configuration and handlers.packages/agent-harness/src/slack-socket.ts-40-75 (1)
40-75: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle Socket Mode control frames before envelope validation. Slack sends
helloanddisconnectmessages withoutenvelope_id, soparseSocketMessagewill reject them, triggering error handling on connect and ignoring the reconnect hint ondisconnect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-harness/src/slack-socket.ts` around lines 40 - 75, Update handleSlackSocketMessage and parseSocketMessage to recognize Slack Socket Mode control frames before requiring an envelope_id or payload. Handle hello normally without recording or acknowledging, and handle disconnect by preserving its reconnect hint through the existing socket lifecycle; keep envelope validation, recording, and acknowledgement unchanged for regular event envelopes..github/workflows/ci.yml-128-128 (1)
128-128: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
actions/checkoutdoesn't disable credential persistence.Both the
container(line 128) andpromote(line 256) jobs check out the repo withoutpersist-credentials: false, while holdingpackages: write/attestations: writetokens for the rest of the job (includingdocker buildon the checked-out context). zizmor flags this as credential persistence via artifacts (artipacked); the persisted token remains in.git/configfor the whole job.🔒 Proposed fix
- uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 + with: + persist-credentials: falseApply to both the
containerjob (line 128) andpromotejob (line 256).Also applies to: 256-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 128, Update the actions/checkout steps in both the container and promote jobs to disable credential persistence by configuring persist-credentials to false, while preserving their existing pinned action references and job behavior.Source: Linters/SAST tools
.github/workflows/ci.yml-151-173 (1)
151-173: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftImage is pushed to GHCR before the vulnerability scan can block it.
On
pushevents,docker/build-push-actionruns withpush: true(lines 151-166) and publishes thestaging-*tag to GHCR immediately. The Trivy scan (167-173) only runs afterward, scanning the already-published image. Since GHCR visibility is set once at the package level (per this PR's README changes), a HIGH/CRITICAL-vulnerable image is briefly publicly pullable before the job fails and blocks promotion.Consider building with
push: false, load: truefirst, scanning the local image, and only pushing (or re-running the push step) once Trivy passes — mirroring what's already done forpull_requestbuilds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 151 - 173, Update the build-and-scan flow around the docker/build-push-action step and “Scan built image with Trivy” step so push events build and load the image locally without publishing it first. Run Trivy against that local image, then add or reuse a subsequent push step that publishes the image only after the scan succeeds; preserve the existing pull_request behavior and image tag.scripts/install-homelab.sh-209-217 (1)
209-217: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAbort the gateway readiness probe after a short timeout.
fetch("http://127.0.0.1:3002/ready")can hang indefinitely and block the installer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-homelab.sh` around lines 209 - 217, Update the gateway_authentication readiness probe to enforce a short timeout on the fetch request to http://127.0.0.1:3002/ready, using the existing Node.js execution block and preserving its current authenticated/authentication_required output and failure handling.packages/database/src/bootstrap-connectors.ts-104-105 (1)
104-105: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCanonicalize JSON before comparing connector configuration.
PostgreSQL
jsonbreorders object keys, soJSON.stringify(existing.record.configuration)can differ from the newly built but equivalentpublicConfiguration. This makes every bootstrap appear changed, producing needless updates, audit events, and outbox work on each startup.Proposed fix
-function sameJson(left: unknown, right: unknown) { - return JSON.stringify(left) === JSON.stringify(right); +function canonicalJson(value: unknown): unknown { + if (Array.isArray(value)) return value.map(canonicalJson); + if (value && typeof value === "object") + return Object.fromEntries( + Object.entries(value) + .sort(([left], [right]) => left.localeCompare(right)) + .map(([key, item]) => [key, canonicalJson(item)]), + ); + return value; +} + +function sameJson(left: unknown, right: unknown) { + return ( + JSON.stringify(canonicalJson(left)) === + JSON.stringify(canonicalJson(right)) + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/src/bootstrap-connectors.ts` around lines 104 - 105, Update sameJson to canonicalize both JSON values before comparing them, using deterministic object-key ordering while preserving array order and nested structures. Ensure equivalent configurations compare equal even when key insertion order differs, so bootstrap avoids unnecessary updates and related side effects.packages/database/migrations/meta/0018_snapshot.json-10280-10290 (1)
10280-10290: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse a UUID foreign key for task agent runs.
tasks.agent_run_idistext, whileagent_runs.idisuuid, and no foreign key connects them. This permits malformed or orphaned references and can break worker/status joins. Change the column touuidand add the appropriate organisation-scoped foreign key.Also applies to: 1106-1116, 1423-1436
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/migrations/meta/0018_snapshot.json` around lines 10280 - 10290, Update the task schema metadata for agent_run_id in the affected snapshot sections to use uuid instead of text, and add the organisation-scoped foreign key referencing agent_runs.id. Ensure the corresponding snapshot entries at the other affected locations remain consistent with the updated column type and relationship.packages/database/migrations/meta/0018_snapshot.json-1118-1122 (1)
1118-1122: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEnforce organisation equality in foreign keys.
These rows store
organisation_id, but the foreign keys validate only the referenced UUID. A row in organisation A can therefore reference an agent, integration, or related record belonging to organisation B. Add composite(organisation_id, id)references or an equivalent database-level invariant for all organisation-scoped relationships.Also applies to: 1423-1449, 5232-5248, 5391-5443
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/migrations/meta/0018_snapshot.json` around lines 1118 - 1122, Update the organisation-scoped foreign-key definitions represented in snapshot 0018, including the additional referenced ranges, so each relationship validates the composite (organisation_id, id) pair rather than only the related UUID. Apply this consistently to agent, integration, and related organisation-scoped records while preserving existing non-scoped foreign keys.packages/database/migrations/meta/0018_snapshot.json-3533-3537 (1)
3533-3537: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd foreign keys for approval references.
decisions.approval_idandreaction_pack_revisions.approval_idare UUID references but have no FK toapprovals.id. Invalid or deleted approval IDs can therefore be persisted, weakening governance provenance. Add the foreign keys and enforce organisation consistency.Also applies to: 3570-3610, 7559-7563, 7660-7712
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/migrations/meta/0018_snapshot.json` around lines 3533 - 3537, Add foreign-key constraints for decisions.approval_id and reaction_pack_revisions.approval_id referencing approvals.id, and enforce matching organisation IDs through the existing organisation-consistency constraint pattern. Update all corresponding schema and migration metadata entries, including the approval reference definitions and table foreign-key sections, while preserving the UUID and nullable properties.packages/database/migrations/meta/0019_snapshot.json-315-341 (1)
315-341: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEnforce organisation consistency in foreign keys.
These child tables carry
organisation_id, but their foreign keys validate only the referenced UUID. PostgreSQL therefore permits, for example, anagent_definitionsrow in organisation A to reference an actor from organisation B, breaking tenant isolation. Add composite(organisation_id, id)references and composite foreign keys across these relationships.Also applies to: 488-540, 1423-1488, 1801-1853, 9348-9400
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/migrations/meta/0019_snapshot.json` around lines 315 - 341, Update the migration snapshot foreign-key definitions for agent_definitions and the other referenced ranges to enforce tenant consistency: include organisation_id alongside the referenced id in each parent key, and define child foreign keys over the corresponding (organisation_id, id) column pairs. Apply this consistently to every relationship identified by the comment, preserving the existing constraint names and delete/update behavior.packages/database/migrations/meta/0020_snapshot.json-7128-7142 (1)
7128-7142: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScope outbox idempotency by organisation.
outbox_idempotency_uniqueis globally unique onidempotency_key, unlike the organisation-scoped idempotency constraints used elsewhere in this snapshot. Identical client keys from different organisations will collide and one tenant can prevent another tenant’s event from being persisted.Change the source index to
(organisation_id, idempotency_key)and regenerate the snapshot.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/migrations/meta/0020_snapshot.json` around lines 7128 - 7142, The outbox_idempotency_unique index is missing organisation scoping. Update its source definition to use the composite columns organisation_id and idempotency_key, then regenerate the database snapshot so the index metadata reflects the new scoped uniqueness.packages/database/migrations/meta/0020_snapshot.json-11046-11054 (1)
11046-11054: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse canonical table identifiers in the provenance constraint.
The check allows
researchWatchlists,reportManifests,reportSchedules, andagentMemories, while the snapshot defines the corresponding tables asresearch_watchlists,report_manifests,report_schedules, andagent_memories. Cleanup manifests using the actual table names will be rejected.Normalize these values in the source schema/migration and regenerate the snapshot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/migrations/meta/0020_snapshot.json` around lines 11046 - 11054, Update the synthetic_artifact_provenance artifact-table constraint in the source schema/migration to use the canonical snake_case identifiers research_watchlists, report_manifests, report_schedules, and agent_memories, then regenerate the corresponding snapshot so its synthetic_artifact_provenance_table_check matches the source.packages/database/migrations/meta/0020_snapshot.json-315-341 (1)
315-341: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEnforce organisation-aware foreign keys.
These relationships constrain only the referenced UUID, not the row’s
organisation_id. A row in organisation A can therefore reference an actor, agent, or Slack installation belonging to organisation B, undermining tenant isolation and allowing inconsistent joins.Add composite organisation-aware foreign keys (with matching parent uniqueness) or an equivalent database-level constraint in the source schema, then regenerate this snapshot. Apply the same audit to all organisation-scoped relationships.
Also applies to: 1423-1488, 10150-10202, 10315-10367
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/migrations/meta/0020_snapshot.json` around lines 315 - 341, Update the source schema for organisation-scoped relationships so foreign keys validate both the referenced UUID and organisation_id, using matching composite parent uniqueness or an equivalent database constraint. Apply this to the agent_definitions relationships and the additional actor, agent, and Slack installation relationships covered by the review. Regenerate the migration snapshot after updating the schema, preserving tenant isolation across all such joins.deploy/docker/docker-compose.homelab.yml-141-146 (1)
141-146: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHardcoded
unifi.localIP has no override; will misroute for most homelab networks.Both
workerandagent-gatewaypinextra_hosts: ["unifi.local:192.168.1.1"]with no corresponding environment variable in.env.homelab.example(onlyUNIFI_BASE_URL/UNIFI_API_KEY/etc. are provided, none of which control this mapping).192.168.1.1is a common default LAN gateway address but not universal — any operator whose UniFi controller/gateway sits at a different address gets this DNS override silently pointing at the wrong host, breaking the UniFi grounding connector by default.Parameterize this via a new env var with a documented default so operators can override it without hand-editing the compose file.
🛠️ Suggested fix
worker: <<: *muster-service extra_hosts: - - "unifi.local:192.168.1.1" + - "unifi.local:${UNIFI_HOST_IP:-192.168.1.1}"agent-gateway: <<: *muster-service extra_hosts: - - "unifi.local:192.168.1.1" + - "unifi.local:${UNIFI_HOST_IP:-192.168.1.1}"Add
UNIFI_HOST_IP=(with a comment on how to find the correct address) todeploy/docker/.env.homelab.example.Also applies to: 165-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/docker/docker-compose.homelab.yml` around lines 141 - 146, Parameterize the hardcoded unifi.local mapping in both the worker and agent-gateway extra_hosts entries using a new UNIFI_HOST_IP environment variable, preserving a documented default fallback. Add UNIFI_HOST_IP to deploy/docker/.env.homelab.example with a comment explaining how operators can determine the correct UniFi host address.deploy/docker/.env.homelab.example-26-26 (1)
26-26: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
SLACK_REDIRECT_URIshould default to anhttps://URLSlack rejects
http://redirect URLs, so this placeholder will make the OAuth flow fail if copied as-is. Use anhttps://example and note that it must be served behind an HTTPS reverse proxy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/docker/.env.homelab.example` at line 26, Update the SLACK_REDIRECT_URI placeholder to use an https:// URL instead of http://, and add a concise note that the endpoint must be served behind an HTTPS reverse proxy.packages/evidence/src/object-storage.ts-118-126 (1)
118-126: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSilent fallback to hard-coded development credentials.
OBJECT_STORAGE_ACCESS_KEY/OBJECT_STORAGE_SECRET_KEYdefault tomuster/local-minio-secretand the endpoint to loopback. In a misconfigured deployment this does not fail fast — it authenticates with a credential literal that is committed to the repository, and failures surface later as opaque 403s. Given this package is used by the immutable homelab release path, require the credentials outside an explicitly local profile.🔒 Sketch
function storageConfiguration() { + const secretKey = process.env.OBJECT_STORAGE_SECRET_KEY; + if (!secretKey && process.env.NODE_ENV === "production") { + throw new Error("OBJECT_STORAGE_SECRET_KEY is required"); + } return { endpoint: process.env.OBJECT_STORAGE_ENDPOINT ?? "http://127.0.0.1:9000", bucket: process.env.OBJECT_STORAGE_BUCKET ?? "muster-evidence", region: process.env.OBJECT_STORAGE_REGION ?? "us-east-1", accessKey: process.env.OBJECT_STORAGE_ACCESS_KEY ?? "muster", - secretKey: process.env.OBJECT_STORAGE_SECRET_KEY ?? "local-minio-secret", + secretKey: secretKey ?? "local-minio-secret", }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evidence/src/object-storage.ts` around lines 118 - 126, Update storageConfiguration to require OBJECT_STORAGE_ACCESS_KEY and OBJECT_STORAGE_SECRET_KEY when running outside the explicitly local profile, rather than falling back to committed development credentials; preserve the local defaults only for that profile and ensure missing production credentials fail fast during configuration.packages/database/src/synthetic-cleanup.ts-140-164 (1)
140-164: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
localeComparemakes canonicalisation locale/ICU dependent, so digests are not portable.
canonical()sorts object keys withlocaleCompareandsyntheticCleanupTableDigestsorts rows the same way.String.prototype.localeComparedepends on the runtime's default locale and ICU version, so a manifest captured byapps/webcan hash differently in the worker or in another container image/locale — and the mismatch surfaces as "digest mismatch"/"pre-state digest changed" on an otherwise valid manifest. Digest input ordering must be deterministic (code-unit comparison), independent of environment. NoteassertExactRows(Line 260) already sorts IDs with the default code-unitArray#sort, so the two orderings can also disagree with each other today.🔒 Proposed fix
+function compare(left: string, right: string) { + return left < right ? -1 : left > right ? 1 : 0; +} + function canonical(value: unknown): string { if (value instanceof Date) return JSON.stringify(value.toISOString()); if (value === null || typeof value !== "object") return JSON.stringify(value); if (Array.isArray(value)) return `[${value.map(canonical).join(",")}]`; return `{${Object.entries(value as Record<string, unknown>) - .sort(([a], [b]) => a.localeCompare(b)) + .sort(([a], [b]) => compare(a, b)) .map(([key, entry]) => `${JSON.stringify(key)}:${canonical(entry)}`) .join(",")}}`; } @@ export function syntheticCleanupTableDigest(rows: ReadonlyArray<CandidateRow>) { return sha256( - canonical([...rows].sort((left, right) => left.id.localeCompare(right.id))), + canonical([...rows].sort((left, right) => compare(left.id, right.id))), ); }
objectInventoryDigest(Line 1362) needs the same change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/src/synthetic-cleanup.ts` around lines 140 - 164, Replace locale-dependent localeCompare ordering with deterministic code-unit ordering in canonical object-key sorting and syntheticCleanupTableDigest row sorting; use the same default Array#sort ordering already used by assertExactRows. Apply the equivalent ordering change to objectInventoryDigest as well, preserving all existing digest inputs and hashing behavior.packages/evidence/src/object-storage.ts-152-297 (1)
152-297: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNo timeout or abort signal on any of the object-storage
fetchcalls.
putObject,getObject,headObject,getObjectVersionanddeleteObjectall issue unbounded requests to an external endpoint. A stalled MinIO/S3 connection pins the calling request or worker task indefinitely — and these run inside the synthetic-cleanup deletion path and evidence upload path.checkObjectStoragealready threads anAbortSignal; the same treatment (orAbortSignal.timeout(...)) is needed here.🛡️ Sketch
+const requestTimeoutMs = Number( + process.env.OBJECT_STORAGE_TIMEOUT_MS ?? 10_000, +); + export const defaultObjectStorage: CleanupObjectStorage = { async putObject(object) { @@ const response = await fetch(url, { method: "PUT", headers: signingHeaders(/* … */), body: Buffer.from(object.body), + signal: AbortSignal.timeout(requestTimeoutMs), });Apply the same
signalto the remaining four operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evidence/src/object-storage.ts` around lines 152 - 297, Update the fetch calls in defaultObjectStorage.putObject, getObject, headObject, getObjectVersion, and deleteObject to include a bounded AbortSignal, reusing the existing object-storage timeout or signal pattern established by checkObjectStorage. Ensure every external request is cancellable and cannot remain stalled indefinitely, while preserving the current request behavior and error handling.
🟡 Minor comments (10)
docs/synthetic-cleanup.md-12-12 (1)
12-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify the cleanup-proof wording.
Replace “are never cleanup proof” with “are never proof of cleanup” to make the safety requirement unambiguous.
Proposed fix
- idempotency-key text are never cleanup proof. Record provenance when a seed, + idempotency-key text are never proof of cleanup. Record provenance when a seed,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/synthetic-cleanup.md` at line 12, In the cleanup-proof wording within the synthetic cleanup documentation, replace “are never cleanup proof” with “are never proof of cleanup,” preserving the surrounding provenance and safety guidance unchanged.Source: Linters/SAST tools
apps/web/components/slack-settings-view.tsx-592-605 (1)
592-605: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKill-switch link points at an arbitrary agent.
settings.agents[0]is whichever agent the settings query happened to return first, so the "kill switch" link sends the operator to an unrelated agent's learning page. Link to the agents index instead, or render the link per exposure row where the agent is known.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/components/slack-settings-view.tsx` around lines 592 - 605, Update the emergency execution control text in the settings view so its kill-switch link targets the agents index rather than using the arbitrary settings.agents[0] agent. Remove the conditional agent-specific href while preserving the existing fallback text and link styling.apps/web/app/api/v1/slack/oauth/callback/route.ts-45-49 (1)
45-49: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFail fast when
SLACK_REDIRECT_URIis unset.Passing
""sends a mismatchedredirect_urito Slack and surfaces as an opaque OAuth parse failure. Throw the same "Slack OAuth is not configured" error the install route uses.🛠️ Proposed fix
+ const redirectUri = process.env.SLACK_REDIRECT_URI; + if (!redirectUri) throw new Error("Slack OAuth is not configured"); const installation = await slack.install( subject, code, - process.env.SLACK_REDIRECT_URI ?? "", + redirectUri, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/api/v1/slack/oauth/callback/route.ts` around lines 45 - 49, Update the Slack OAuth callback flow around slack.install to validate SLACK_REDIRECT_URI before invoking the installation. When the environment variable is unset, fail immediately with the same “Slack OAuth is not configured” error and behavior used by the install route; otherwise pass the configured URI unchanged instead of defaulting to an empty string.scripts/install-homelab.sh-219-226 (1)
219-226: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnescaped admin email/password in JSON payload.
admin_password/admin_emailare interpolated directly into a hand-built JSON string. A customMUSTER_LOCAL_ADMIN_PASSWORDcontaining"or\breaks the payload and fails the automated bootstrap signup.🐛 Proposed fix
+signup_payload="$(jq -n --arg email "$admin_email" --arg password "$admin_password" \ + '{name: "Muster Administrator", email: $email, password: $password}')" signup_status="$( curl --silent \ --output /dev/null \ --write-out '%{http_code}' \ -H "content-type: application/json" \ - -d "{\"name\":\"Muster Administrator\",\"email\":\"${admin_email}\",\"password\":\"${admin_password}\"}" \ + -d "$signup_payload" \ "${base_url}/api/auth/sign-up/email" )"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-homelab.sh` around lines 219 - 226, The sign-up request in the signup_status curl block builds invalid JSON when admin_email or admin_password contains JSON-special characters. Replace the hand-built payload with safe JSON serialization using the script’s available tooling, ensuring both values are escaped correctly while preserving the existing request fields and endpoint.scripts/install-homelab.sh-84-106 (1)
84-106: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStale
AUTH_TRUSTED_ORIGINSafter changingMUSTER_PUBLIC_URLalone.If
.env.homelabalready exists and a rerun sets a newMUSTER_PUBLIC_URLwithout also settingAUTH_TRUSTED_ORIGINS, the persistedexisting_originsis reused verbatim (lines 102-103) and may not include the new public URL, breaking Better Auth on the new origin.🐛 Proposed fix
elif [[ "$environment_created" == "false" && -n "$existing_origins" ]]; then - requested_origins="$existing_origins" + if [[ ",$existing_origins," == *",$requested_public_url,"* ]]; then + requested_origins="$existing_origins" + else + requested_origins="${existing_origins},${requested_public_url}" + fi else🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-homelab.sh` around lines 84 - 106, Update the AUTH_TRUSTED_ORIGINS selection logic near requested_origins so changing MUSTER_PUBLIC_URL causes the generated origins to use the new requested_public_url instead of blindly reusing existing_origins. Preserve explicitly provided AUTH_TRUSTED_ORIGINS, and retain existing persisted origins when MUSTER_PUBLIC_URL was not overridden..github/workflows/ci.yml-211-213 (1)
211-213: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"Verify container starts" doesn't actually start anything.
The step is named to verify the container starts, but only runs
docker image inspect(a metadata check on the already-loaded image), which cannot catch an entrypoint crash or startup failure.♻️ Proposed fix
- name: Verify container starts if: github.event_name == 'pull_request' - run: docker image inspect "${{ steps.image.outputs.build_ref }}" + run: | + docker run --rm --detach --name muster-pr-smoke "${{ steps.image.outputs.build_ref }}" + sleep 5 + docker logs muster-pr-smoke + docker inspect --format '{{.State.Running}}' muster-pr-smoke | grep -x true + docker rm --force muster-pr-smoke🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 211 - 213, Update the “Verify container starts” workflow step to actually launch the built image, replacing the metadata-only docker image inspect check with a command that runs the image and fails when its entrypoint cannot start. Preserve the existing pull_request condition and image reference from steps.image.outputs.build_ref.tests/homelab/install-homelab.test.sh-17-37 (1)
17-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the
opensslstub return distinct values per callThe
curlstub matches the installer’s--write-outusage, but theopensslstub always returns the samesynthetic--hexvalue for everyopenssl rand -hex ...call. That can hide bugs that depend on separate generated secrets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/homelab/install-homelab.test.sh` around lines 17 - 37, Update the openssl stub created in the test setup so each rand -hex invocation returns a distinct synthetic value, rather than deriving the same output from the argument alone. Preserve the existing command logging behavior and ensure repeated calls generate distinguishable secrets for installer validation.packages/database/src/synthetic-cleanup.ts-1973-1993 (1)
1973-1993: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSerialization-failure detection only inspects
error.cause.code.Depending on how the driver error propagates through the Drizzle transaction wrapper,
40001can appear on the error itself rather than oncause. When it does, a concurrent apply that actually committed a receipt surfaces as a raw serialization error to the caller instead of the idempotentapplied: falseresult the concurrent-apply integration test relies on. Check both.🐛 Proposed fix
- const code = - cause && typeof cause === "object" && "code" in cause - ? (cause as { code?: unknown }).code - : undefined; + const codeOf = (value: unknown) => + value && typeof value === "object" && "code" in value + ? (value as { code?: unknown }).code + : undefined; + const code = codeOf(cause) ?? codeOf(error);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/src/synthetic-cleanup.ts` around lines 1973 - 1993, Update the catch block around the synthetic cleanup transaction to detect serialization code "40001" from either the top-level error or its cause, while preserving the existing receipt lookup and idempotent applied:false return. Only rethrow when neither location contains that code.apps/web/lib/alfie-research-domain.ts-93-97 (1)
93-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude archived watchlists from the name conflict
list()hides archived rows, butcreate()still conflicts on the plain(organisationId, name)unique index. An archived watchlist will keep returning a misleading 409 for a name users can no longer see or reuse.apps/web/lib/alfie-research-domain.ts:87-146🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/alfie-research-domain.ts` around lines 93 - 97, Update the watchlist name-conflict query in create() to exclude archived rows by applying the same archivedAt-is-null condition used by list(). Ensure archived watchlists do not trigger the organisationId/name conflict, while active watchlists retain the existing 409 behavior.packages/integrations/src/governed.ts-75-87 (1)
75-87: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winAdd a test for the custom CA pinning path
packages/integrations/src/governed.test.tsshould cover a request withtlsCaCertificateBase64so thecaoption onpinnedJsonRequestis exercised.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/integrations/src/governed.ts` around lines 75 - 87, Add a test in governed.test.ts covering a governed request configured with tlsCaCertificateBase64, and assert that pinnedJsonRequest receives the decoded certificate through its ca option. Reuse the existing request and mocking patterns in the test file.
|
|
||
| const slackSocketAbort = new AbortController(); | ||
| let slackSocketTask: Promise<void> | undefined; | ||
| if (process.env.SLACK_SOCKET_MODE_ENABLED === "true") { | ||
| const appToken = process.env.SLACK_APP_TOKEN; | ||
| if (!appToken) | ||
| throw new Error( | ||
| "SLACK_APP_TOKEN is required when SLACK_SOCKET_MODE_ENABLED=true", | ||
| ); | ||
| const adapter = new SlackGovernanceAdapter(); | ||
| slackSocketTask = runSlackSocketMode({ | ||
| appToken, | ||
| signal: slackSocketAbort.signal, | ||
| recordEnvelope: (envelope) => adapter.recordSocketEnvelope(envelope), | ||
| onError: (error) => | ||
| jsonLog("error", "slack.socket.error", { | ||
| error: error instanceof Error ? error.message : "Socket Mode failed", | ||
| }), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unhandled rejection: nothing observes slackSocketTask until shutdown.
runSlackSocketMode rejects on a non-xapp- token (and on any escape from its loop), but the returned promise has no rejection handler attached at creation time, so Node raises an unhandled rejection and terminates the worker. The same promise is then awaited unguarded during shutdown (Line 2211), where a rejection aborts the remaining worker/queue/database cleanup.
🛡️ Proposed fix
- slackSocketTask = runSlackSocketMode({
+ slackSocketTask = runSlackSocketMode({
appToken,
signal: slackSocketAbort.signal,
recordEnvelope: (envelope) => adapter.recordSocketEnvelope(envelope),
onError: (error) =>
jsonLog("error", "slack.socket.error", {
error: error instanceof Error ? error.message : "Socket Mode failed",
}),
- });
+ }).catch((error: unknown) => {
+ jsonLog("error", "slack.socket.stopped", {
+ error: error instanceof Error ? error.message : "Socket Mode failed",
+ });
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const slackSocketAbort = new AbortController(); | |
| let slackSocketTask: Promise<void> | undefined; | |
| if (process.env.SLACK_SOCKET_MODE_ENABLED === "true") { | |
| const appToken = process.env.SLACK_APP_TOKEN; | |
| if (!appToken) | |
| throw new Error( | |
| "SLACK_APP_TOKEN is required when SLACK_SOCKET_MODE_ENABLED=true", | |
| ); | |
| const adapter = new SlackGovernanceAdapter(); | |
| slackSocketTask = runSlackSocketMode({ | |
| appToken, | |
| signal: slackSocketAbort.signal, | |
| recordEnvelope: (envelope) => adapter.recordSocketEnvelope(envelope), | |
| onError: (error) => | |
| jsonLog("error", "slack.socket.error", { | |
| error: error instanceof Error ? error.message : "Socket Mode failed", | |
| }), | |
| }); | |
| } | |
| const slackSocketAbort = new AbortController(); | |
| let slackSocketTask: Promise<void> | undefined; | |
| if (process.env.SLACK_SOCKET_MODE_ENABLED === "true") { | |
| const appToken = process.env.SLACK_APP_TOKEN; | |
| if (!appToken) | |
| throw new Error( | |
| "SLACK_APP_TOKEN is required when SLACK_SOCKET_MODE_ENABLED=true", | |
| ); | |
| const adapter = new SlackGovernanceAdapter(); | |
| slackSocketTask = runSlackSocketMode({ | |
| appToken, | |
| signal: slackSocketAbort.signal, | |
| recordEnvelope: (envelope) => adapter.recordSocketEnvelope(envelope), | |
| onError: (error) => | |
| jsonLog("error", "slack.socket.error", { | |
| error: error instanceof Error ? error.message : "Socket Mode failed", | |
| }), | |
| }).catch((error: unknown) => { | |
| jsonLog("error", "slack.socket.stopped", { | |
| error: error instanceof Error ? error.message : "Socket Mode failed", | |
| }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/worker/src/index.ts` around lines 2152 - 2171, Attach an immediate
rejection handler to the promise created by runSlackSocketMode in the
slackSocketTask initialization, logging the failure without creating an
unhandled rejection; retain the task for shutdown tracking. Update shutdown’s
await of slackSocketTask to catch or otherwise contain rejection so cleanup
continues even when the socket loop fails.
2 participants
Outcome
Lands the combined core platform release:
Runtime boundaries
Connector evidence is organisation-scoped, capability checked, encrypted at rest, persisted as query runs, audit logged, emitted through the outbox, bounded, and treated as untrusted evidence.
Validation
Closes #19
Closes #33
Closes #40
Summary by CodeRabbit
New Features
Bug Fixes
Documentation